Skip to content

Bound IINC increment to signed short#526

Merged
garydgregory merged 1 commit into
apache:masterfrom
rootvector2:iinc-increment-bound
Jul 21, 2026
Merged

Bound IINC increment to signed short#526
garydgregory merged 1 commit into
apache:masterfrom
rootvector2:iinc-increment-bound

Conversation

@rootvector2

Copy link
Copy Markdown
Contributor

IINC.setIncrement stores the increment without checking its range, but dump writes the wide-form increment with writeShort. The wide iinc increment is a signed short, so new IINC(0, 40000) (or setIncrement with any value outside -32768..32767) is accepted and getIncrement still returns 40000, yet dump emits -25536. Found while auditing the operand setters around the earlier setIndex bounds fix; the increment is the one that was left unchecked.

Reject an out-of-range increment in setIncrement with the same isValidShort guard BranchInstruction.dump and the sibling setIndex already use. The two-arg constructor routes through setIncrement, so checking it there covers every construction path.

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body.

`IINC.setIncrement` stored the increment without checking its range, but `dump` writes the wide-form increment with `writeShort`. A value outside the signed-short range was accepted and then truncated to a different value on dump. Reject it in `setIncrement`, which the constructor routes through too.
@garydgregory garydgregory changed the title bound iinc increment to signed short Bound IINC increment to signed short Jul 20, 2026
@garydgregory

Copy link
Copy Markdown
Member

Hello @rootvector2
The JVM specification says const is "an immediate signed byte". Where did you get a "short"?

@rootvector2

Copy link
Copy Markdown
Contributor Author

the wide form. plain iinc takes a signed byte, but setWide() upgrades the instruction to wide iinc whenever the increment doesn't fit in a byte, and dump then writes it with writeShort. per JVMS 6.5 wide (format 2) the extended const is a signed 16-bit value, so that's the widest range IINC can encode. the byte bound needs no guard since the upgrade to wide is automatic; past the short bound writeShort truncates, which is how new IINC(0, 40000) ends up dumping an increment of -25536.

@rootvector2

Copy link
Copy Markdown
Contributor Author

That's the byte-form iinc. Once the increment doesn't fit in a byte, setWide() switches to the wide form, and wide iinc takes a signed 16-bit const (constbyte1/constbyte2, second form of wide: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.wide), which dump writes with writeShort(c). So a signed short is the widest increment BCEL can emit; anything beyond it gets silently truncated by writeShort, which is what the guard rejects.

@garydgregory
garydgregory merged commit 68698bb into apache:master Jul 21, 2026
21 of 22 checks passed
@garydgregory

Copy link
Copy Markdown
Member

@rootvector2 ty merged 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants